[CDAP-21269] Add support for Dataproc Flexible Machine Types in compute profiles - #16204
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for Dataproc Instance Flexibility Policies by adding configuration options for master and worker flex VM machine types. However, the current implementation is prone to throwing a NullPointerException at runtime because Protobuf builders do not accept null values when these optional properties are omitted. Feedback has been provided to conditionally build and safely apply these flexibility policies, as well as to support comma-separated machine types.
123-komal
changed the title
da0c03c to
c22ed75
Compare
035da77 to
03ed2d9
Compare
| return Strings.isNullOrEmpty(val) | ||
| ? Collections.emptyList() | ||
| : Collections.unmodifiableList( | ||
| Splitter.on(',').trimResults().omitEmptyStrings().splitToList(val)); |
There was a problem hiding this comment.
Splitter.splitToList returns immutable list. No need to wrap it again.
private static final Splitter COMMA_SPLITTER =
Splitter.on(',').trimResults().omitEmptyStrings();
private static List<String> getStringList(Map<String, String> properties, String key) {
String val = getString(properties, key);
return Strings.isNullOrEmpty(val)
? List.of()
: COMMA_SPLITTER.splitToList(val);
}There was a problem hiding this comment.
I tried using the same method you provide, but the List.of() is not supported with the given java version. I think it was introduced in the later ones so I used the below one
private static final Splitter COMMA_SPLITTER =
Splitter.on(',').trimResults().omitEmptyStrings();
private static List<String> getStringList(Map<String, String> properties, String key) {
String val = getString(properties, key);
return Strings.isNullOrEmpty(val)
? Collections.emptyList()
: COMMA_SPLITTER.splitToList(val);
}
| "n1", | ||
| "n2", | ||
| "n2d", | ||
| "e2" |
There was a problem hiding this comment.
Should n4 be present in the list?
There was a problem hiding this comment.
I have kept the supported option same as the one with we listed in masterMachineType. But we can still n4 during runtime, which I also tested and it worked.
| * Parses a comma-separated string property into a trimmed list of strings, | ||
| * or returns an empty list if null/empty. | ||
| */ | ||
| private static List<String> getStringList(Map<String, String> properties, String key) { |
There was a problem hiding this comment.
Can we check other fields that are "widget-type": "csv"
Example : networkTags ,scopes etc..
How are they parsed ?
And if this function can be made generic to be used for other such fields?
There was a problem hiding this comment.
I checked the other "widget-type": "csv" fields (networkTags, scopes, and initActions), updated the logic to use the generalized getString method.
There was a problem hiding this comment.
I kept the logic for initActions change as it is, as it doesn't seems to backward compatible.
There was a problem hiding this comment.
Reverted the initActions to the original change.
03ed2d9 to
22dbc60
Compare
c0d5a8b to
6e30e50
Compare
6e30e50 to
e95d026
Compare
Add support for Dataproc Flexible Machine Types in compute profiles Adding Support for the Flexvm Updated Updated Updated Updated Updated Updated Updated Updated Updated Updated Updated
e95d026 to
49b4f43
Compare
|
123-komal
changed the title
3 participants
Adds support for configuring flexible fallback machine types for Dataproc master and worker nodes. This allows clusters to automatically fall back to alternative machine types in priority order if the primary machine type is unavailable.
Changes
Testing